[AMD] CanonicalizePointers: Handle different base pointers and offsets#9541
[AMD] CanonicalizePointers: Handle different base pointers and offsets#9541kelesvol wants to merge 1 commit intotriton-lang:mainfrom
Conversation
kelesvol
requested review from
antiagainst,
ptillet and
zhanglx13
as code owners
|
cc @AlexAUT |
AlexAUT
left a comment
AlexAUT
left a comment
There was a problem hiding this comment.
Thanks for fixing this, looks reasonable to me. cc @yangshuxin since he also touched this pass IIRC.
| return !(lhs == rhs); | ||
| } | ||
|
|
||
| static FatPtrAttrs merge(const FatPtrAttrs &lhs, const FatPtrAttrs &rhs) { |
There was a problem hiding this comment.
Would intersect be a better name? merge sounds a bit like we take the union of all attributes.
| lhs.attributes == rhs.attributes && | ||
| lhs.smallTensorBase == rhs.smallTensorBase; | ||
| lhs.isSmallTensor == rhs.isSmallTensor && | ||
| lhs.attributes == rhs.attributes; |
There was a problem hiding this comment.
if ptr1 and ptr2 is from small tensor a and b respectively, they share the same attribute, then == return true?.
I think this is fundamentally wrong although replacing smallTensorBase with a boolean does not expose bug.
Maybe you can add another field to indicate the case where the base is dynamic, it is a combination of multiple real tensors. say, introduce dynamic-base for that purpose.
When merging from different base, clobber the smallTensorBase, and set the dynamic-base.
Or conservertively, just clobber of tensorBase to indicate it too complicated to handle.
There was a problem hiding this comment.
if ptr1 and ptr2 is from small tensor a and b respectively, they share the same attribute, then == return true?
No, we compare attributes here, not the actual pointers.
Maybe you can add another field to indicate the case where the base is dynamic, it is a combination of multiple real tensors. say, introduce dynamic-base for that purpose.
Where will we use dynamic base in this case? Currently, we keep smallTensorBase, but don't use it other than checking if it's a small tensor. With isSmallTensor added, we'll no longer need that either.
There was a problem hiding this comment.
No, we compare attributes here, not the actual pointers.
Then, why it's valid? Two pointers are equal if the share the same attribute even if they don't have the same base?
While I cannot give your examples as to why this will have incur problem at this moment (need to check the code), does not your change sound very dangerous? You claim two pointers are equals without comparing their base-pointer. It might works for now because, by current implementation, when two pointers involved into comparision, they may happen to have to same bases. It has huge potential problems.
We cannot weaken the condition at the huge risk of correctness just to make dealing dynamic-base slightly easier.
| for (const auto &attr : lhs.attributes) { | ||
| auto it = rhs.attributes.find(attr.first); | ||
| if (it != rhs.attributes.end() && it->second == attr.second) | ||
| result.attributes[attr.first] = attr.second; |
There was a problem hiding this comment.
maybe you can replace the data-type to small vectormap?
| // If the fat-pointer points to somewhere in a small-tensor, keep track the | ||
| // base of the tensor. | ||
| Value smallTensorBase; | ||
| bool isSmallTensor = false; |
There was a problem hiding this comment.
nope, we cannot say ptr-to-a == ptr-to-b.
There was a problem hiding this comment.
Same as above, this is a part of the attributes, it doesn't mean pointers are the same.
`scf.if` and `arith.select` ops can have different base pointers accross branches. In that case, `FatPtrAttrs` can't keep `smallTensorBase` or pick one of the branches. Instead, replace `smallTensorBase` (Value) with `isSmallTensor` (bool) in `FatPtrAttrs` and add a conservative `merge` function for combining attrs. Also, handle different scalar offsets properly when handling `arith.select` ops.
3 participants
scf.ifandarith.selectops can have different base pointers accross branches. In that case,FatPtrAttrscan't keepsmallTensorBaseor pick one of the branches. Instead, replacesmallTensorBase(Value) withisSmallTensor(bool) inFatPtrAttrsand add a conservativemergefunction for combining attrs. Also, handle different scalar offsets properly when handlingarith.selectops.